test: cover benchmark dashboard renderer by ndycode · Pull Request #302 · ndycode/codex-multi-auth

ndycode · 2026-03-22T04:44:32Z

Summary

add a focused smoke test for scripts/benchmark-render-dashboard.mjs
verify the script writes HTML from a minimal benchmark summary fixture
improve coverage for a previously untested entrypoint script

Validation

npm run typecheck
npm run lint -- test/benchmark-render-dashboard-script.test.ts
npm run test -- test/benchmark-render-dashboard-script.test.ts
manual QA: node scripts/benchmark-render-dashboard.mjs --input=<fixture> --output=<tmp>/dashboard.html

note: greptile review for oc-chatgpt-multi-auth. cite files like `lib/foo.ts:123`. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

adds a focused smoke-test suite for scripts/benchmark-render-dashboard.mjs, covering the happy path, spaced paths (windows safety), and three error branches (missing input file, malformed json, missing output dir). the test follows project conventions correctly: removeWithRetry with EBUSY/EPERM/ENOTEMPTY backoff for windows filesystem safety, spawnSync with an explicit 10s timeout, and absolute temp paths via mkdtempSync.

removeWithRetry implementation correctly mirrors the pattern used across repo-hygiene.test.ts, rotation-integration.test.ts, and unified-settings.test.ts
paths-with-spaces test is a good windows coverage addition since the script arg parser uses startsWith+slice rather than shell splitting
the afterEach registered at module level (outside describe) is idiomatic vitest and works correctly here
one missing vitest coverage gap: the no---input branch exits via process.exitCode = 1; return; (not process.exit(1)), so stderr is empty and the .catch() handler never fires — this path has no test case

Confidence Score: 5/5

safe to merge — test-only addition with no production code changes and correct windows safety patterns throughout
single new test file, no production code changes, follows all project conventions (removeWithRetry, absolute paths, spawnSync), the one missing test case for the no-input usage branch is a minor gap and non-blocking
no files require special attention

Important Files Changed

Filename	Overview
test/benchmark-render-dashboard-script.test.ts	new smoke-test suite for the benchmark render dashboard script; correctly uses removeWithRetry for windows safety, covers the main happy path, path-with-spaces, missing-file, malformed-json, and missing-output-dir cases; missing one test case for the no-`--input` usage branch which exits via `process.exitCode` rather than `process.exit`

Sequence Diagram

sequenceDiagram
    participant T as vitest test
    participant S as spawnSync
    participant N as node (child process)
    participant FS as filesystem (tmpdir)

    T->>FS: mkdtempSync → create temp root
    T->>FS: writeFileSync → write summary.json fixture
    T->>S: spawnSync(node, [scriptPath, --input=..., --output=...])
    S->>N: spawn child process (timeout 10s)
    N->>FS: readFile(inputPath)
    N->>N: JSON.parse + renderDashboardHtml()
    N->>FS: writeFile(outputPath, html)
    N-->>S: stdout "Dashboard written:", status 0
    S-->>T: SpawnSyncReturns
    T->>FS: readFileSync(outputPath) → assert html contents
    T->>FS: afterEach removeWithRetry(root, recursive+force)

Prompt To Fix All With AI

This is a comment left during a code review.
Path: test/benchmark-render-dashboard-script.test.ts
Line: 111-113

Comment:
**missing coverage for no-`--input` usage path**

the script's `main()` handles missing `--input` via `process.exitCode = 1; return;` — not `process.exit(1)` — so the `.catch()` handler never fires and `stderr` stays empty. this exit mechanism is subtly different from the error paths that are tested. worth adding a case to verify that omitting `--input` still exits with status 1 and produces expected stdout (usage text), not an error on stderr.

```ts
it("exits with status 1 when --input is not provided", () => {
    const result = runRenderDashboard([]);

    expect(result.status).toBe(1);
    expect(result.stderr).toBe("");
    expect(result.stdout).toContain("Usage:");
});
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "test: harden benchma..."}

coderabbitai · 2026-03-22T04:44:47Z

📝 Walkthrough

Walkthrough

new test file validates the benchmark dashboard script's cli behavior by creating temporary directories, writing test summary data, spawning the process with input/output arguments, and asserting successful execution with expected html output.

Changes

Cohort / File(s)	Summary
Benchmark Dashboard Test `test/benchmark-render-dashboard-script.test.ts`	91 lines added. new vitest suite that spawns the benchmark script as a subprocess, validates exit code and stdout contains "Dashboard written:", and checks generated html contains expected static and dynamic content. uses temporary directories with afterEach cleanup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

review notes

windows compatibility risk: test/benchmark-render-dashboard-script.test.ts:line uses spawnSync with node executable path. verify the path resolution works consistently across windows/unix paths. also check if summary.json fixture handles path separators correctly on windows.

missing regression test coverage: the test validates happy path (successful execution, correct html output) but doesn't cover error cases. consider adding tests for:

malformed summary.json input
missing required meta or rows fields
write permission failures on output directory
invalid --input or --output arguments

concurrency consideration: temporary directory cleanup in afterEach could fail if the dashboard script process hasn't fully released file handles on windows. might need explicit wait or delayed cleanup.

test isolation: ensure temp directories don't accumulate if tests fail. current recursive deletion pattern is solid but verify fs operations have proper error handling in cleanup hook.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	title follows conventional commits format (test: scope summary), type is valid (test), summary is lowercase imperative and under 72 chars.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	PR description covers all critical sections: summary, what changed, validation steps, and manual QA. However, it omits several required template sections (Docs & Governance Checklist, Risk & Rollback).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch test/pr8-benchmark-render-dashboard-smoke

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch test/pr8-benchmark-render-dashboard-smoke

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/benchmark-render-dashboard-script.test.ts`:
- Around line 23-90: Add edge-case tests alongside the existing happy-path in
test/benchmark-render-dashboard-script.test.ts: add one test that spawns the
script (using process.execPath and scriptPath) with a non-existent --input and
asserts non-zero exit and stderr includes a helpful message; add one test that
writes malformed JSON to the inputPath, spawns the script, and asserts non-zero
exit and stderr contains JSON parse or summary error; add one test that points
--output to a file inside a non-existent directory and assert either the script
creates the directory and exits 0 (check stdout "Dashboard written:" and that
file exists) or it exits non-zero with a clear stderr; use the same
mkdtempSync/tempRoots pattern and spawnSync and readFileSync to implement these
checks so they run alongside the existing test.
- Around line 79-86: Update the test that runs spawnSync so failed runs surface
stderr and the process can't hang: when calling spawnSync (the call using
process.execPath and scriptPath), add the timeout option (e.g., timeout: <ms>)
and update the assertions to include or log result.stderr when result.status is
not 0; specifically, include result.stderr in the expect failure message or an
additional expect toContain/length check so error output is visible alongside
the existing expect(result.stdout).toContain("Dashboard written:").
- Line 81: Add a regression test that passes Windows-like paths containing
spaces to the benchmark-render-dashboard script to lock in correct parsing; in
the test file (test/benchmark-render-dashboard-script.test.ts) add a case that
creates a temporary input path with spaces (e.g., using tmp or fs.mkdtemp and a
filename with a space) and invokes spawnSync with the same argument array
pattern ([scriptPath, `--input=${inputPathWithSpaces}`,
`--output=${outputPath}`]) to assert the script
(scripts/benchmark-render-dashboard.mjs) still receives and parses the
`--input=PATH` form correctly (note the script's parser at lines ~18-22 slices
after '='), then verify expected behavior or output.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0f2c8cc1-de5d-43ce-a413-054da3ac93f2

📥 Commits

Reviewing files that changed from the base of the PR and between 1be5e95 and 5bd7739.

📒 Files selected for processing (1)

test/benchmark-render-dashboard-script.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (1)

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

test/benchmark-render-dashboard-script.test.ts

🔇 Additional comments (2)

test/benchmark-render-dashboard-script.test.ts (2)

14-21: cleanup logic looks solid.

the afterEach hook with rmSync({ recursive: true, force: true }) handles leftover temp dirs correctly, and the force flag avoids throws on already-deleted paths. no issues here.

1-12: imports and setup are clean.

using node: prefixed imports is good practice for clarity. tempRoots array at module scope works fine with vitest's isolation model.

test/benchmark-render-dashboard-script.test.ts

test: cover benchmark dashboard renderer

5bd7739

coderabbitai bot requested changes Mar 22, 2026

View reviewed changes

test/benchmark-render-dashboard-script.test.ts Show resolved Hide resolved

test/benchmark-render-dashboard-script.test.ts Outdated Show resolved Hide resolved

test/benchmark-render-dashboard-script.test.ts Outdated Show resolved Hide resolved

test: harden benchmark dashboard script coverage

5c5e80d

coderabbitai bot approved these changes Mar 22, 2026

View reviewed changes

ndycode mentioned this pull request Mar 22, 2026

release: merge current main-target PR set #318

Merged

ndycode merged commit b4af196 into main Mar 23, 2026
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: cover benchmark dashboard renderer#302

test: cover benchmark dashboard renderer#302
ndycode merged 2 commits intomainfrom
test/pr8-benchmark-render-dashboard-smoke

ndycode commented Mar 22, 2026 •

edited by greptile-apps bot

Loading

Uh oh!

coderabbitai bot commented Mar 22, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

review notes

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ndycode commented Mar 22, 2026 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validation

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

coderabbitai bot commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

review notes

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ndycode commented Mar 22, 2026 •

edited by greptile-apps bot

Loading

note: greptile review for oc-chatgpt-multi-auth. cite files like `lib/foo.ts:123`. confirm regression tests + windows concurrency/token redaction coverage.

coderabbitai bot commented Mar 22, 2026 •

edited

Loading